PKCS#11 Ed25519 tests by varder · Pull Request #618 · OP-TEE/optee_test

varder · 2022-09-29T07:50:04Z

Adds tests to import, sign and verify Ed25519, Ed25519ctx and Ed25519ph.

The pull request comes with the following pull requests:
OP-TEE/optee_os#5559
OP-TEE/optee_os#5574

etienne-lms

The pkcs11 part looks good to me (aside the few minor comments)

etienne-lms · 2022-10-04T12:30:38Z

+        .public_len = ARRAY_SIZE(_vect ##_public), \
+		.flag = _flag, \
+		.context = _vect ## _context, \
+		.context_len = ARRAY_SIZE(_vect ##_context), \


indentation

etienne-lms · 2022-10-04T12:33:03Z

+	size_t private_len;
+	const uint8_t *public;
+	size_t public_len;
+	const bool flag;


rename ph_flag as this is the pre-hash flag?

etienne-lms · 2022-10-04T12:48:22Z

+			goto err;
+
+		rv = C_Sign(session, (CK_BYTE_PTR)test->message, test->message_len,
+					 (CK_BYTE_PTR)sign, &sign_len);


indentation (and at line 7972)

etienne-lms · 2022-10-04T12:49:30Z

+
+	CK_ATTRIBUTE public_key_template[] = {
+		{ CKA_CLASS, &(CK_OBJECT_CLASS){ CKO_PUBLIC_KEY }, sizeof(CK_OBJECT_CLASS) },
+		{ CKA_KEY_TYPE,	&(CK_KEY_TYPE){CKK_EC_EDWARDS}, sizeof(CK_KEY_TYPE) },


nit: space char around CKK_EC_EDWARDS: { CKK_EC_EDWARDS }

etienne-lms · 2022-10-04T12:59:14Z

+
+static struct eddsa_test eddsa_sign_tests[] = {
+	CKTEST_EDDSA_TEST(ed25519_params, ed25519_rfc_8032_7_1, 0),
+	CKTEST_EDDSA_TEST(ed25519_params, ed25519_rfc_8032_7_1, 1),


This looks like an extra tests, testing vector 7.1 with pre-hash enabled. The EdDSA RFC mentions this test setup?
Same question at line 7850.

agree, removed

varder · 2022-10-17T05:56:25Z

Hi @etienne-lms,
Could you please have a look at the updates?
Thanks

etienne-lms

Sorry @varder for the late feedback. Error path issues to address.

etienne-lms · 2022-10-17T06:55:03Z

+		ADBG_EXPECT_CK_OK(c, C_DestroyObject(session, public_key));
+	}
+
+err:


there are several things to clean on error case. I suggest:

ADBG_EXPECT_CK_OK(c, C_DestroyObject(session, private_key)); ADBG_EXPECT_CK_OK(c, C_DestroyObject(session, public_key)); } + ADBG_EXPECT_CK_OK(c, C_CloseSession(session)); + ADBG_EXPECT_CK_OK(c, close_lib()); + return; + -err: +err_destroy_keys + ADBG_EXPECT_CK_OK(c, C_DestroyObject(session, private_key)); + ADBG_EXPECT_CK_OK(c, C_DestroyObject(session, public_key)); +err_close_sess: + C_CloseSession(session); +err_close_lib: + close_lib(); }

Done,
Thank for providing the solution

etienne-lms · 2022-10-17T06:55:51Z

+					    ARRAY_SIZE(private_key_template),
+					    &private_key);
+			if (!ADBG_EXPECT_CK_OK(c, rv))
+				goto err;


We should destroy the created persistent (token) object public_key above.

etienne-lms · 2022-10-17T07:11:48Z

+
+		rv = C_SignInit(session, &sign_mechanism, private_key);
+		if (!ADBG_EXPECT_CK_OK(c, rv))
+			goto err;


failing here needs to attempt to destroy created persistent object(s).

etienne-lms · 2022-10-17T07:13:05Z

+			   CKF_SERIAL_SESSION | CKF_RW_SESSION,
+			   NULL, NULL, &session);
+	if (!ADBG_EXPECT_CK_OK(c, rv))
+		goto err;


goto err_close_lib;
and only call close_lib()

varder · 2022-10-20T10:15:55Z

Hi @etienne-lms
Can I squash the commit?

etienne-lms

Can I squash the commit?

Ok.
Acked-by: Etienne Carriere <etienne.carriere@linaro.org>

Adds tests to import, sign and verify ED25519, ED25519ctx and ED25519ph. The PKCS11 Specification: https://docs.oasis-open.org/pkcs11/pkcs11-spec/v3.1/cs01/ pkcs11-spec-v3.1-cs01.pdf Signed-off-by: Valerii Chubar <valerii_chubar@epam.com> Acked-by: Etienne Carriere <etienne.carriere@linaro.org>

varder mentioned this pull request Sep 29, 2022

libckteec: Add EDDSA attribute serialization OP-TEE/optee_client#324

Merged

varder force-pushed the pkcs11_ed25519 branch 2 times, most recently from aa718cd to 986256c Compare October 3, 2022 16:02

etienne-lms reviewed Oct 4, 2022

View reviewed changes

varder mentioned this pull request Oct 7, 2022

ta: pkcs11: Fix key generation for Ed25519 OP-TEE/optee_os#5574

Merged

varder force-pushed the pkcs11_ed25519 branch from 9ee35d5 to 84c3417 Compare October 12, 2022 14:31

etienne-lms reviewed Oct 17, 2022

View reviewed changes

etienne-lms reviewed Oct 20, 2022

View reviewed changes

varder force-pushed the pkcs11_ed25519 branch from 26afe95 to a47b266 Compare October 20, 2022 15:44

jforissier merged commit e4f3cd1 into OP-TEE:master Oct 25, 2022

Conversation

varder commented Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

varder commented Oct 17, 2022

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

varder commented Oct 20, 2022

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

varder commented Sep 29, 2022 •

edited

Loading